Fixes #37861 - Update web UI for multi-CV activation key display #11161
Conversation
| <span id="ak-cve-details" data-ak-details="{{ activationKey }}" class="hidden"></span> | ||
| <foreman-react-component | ||
| name="CVEDetailsCard" | ||
| data-props="" |
There was a problem hiding this comment.
Had to solve a few problems here.
First, foreman-react-component is supposed to be able to take props directly via data-props. It then parses them as JSON and uses them. But I tried giving it data-props="{{ activationKey }}" and got an error about unexpected token in JSON. It was trying to parse "{{ activationKey }}" as JSON, which means that it was somehow getting to the DOM before AngularJS had a chance to evaluate it and replace "{{ activationKey }}" with the activation key's JSON data. So I tried putting the data in a hidden span, and then reading it below in CVEDetailsCard.js with JSON.parse(node.dataset.akDetails).
Now, it works on initial page load. But when I clicked around or used the breadcrumb switcher, the AK card wasn't updating. I found that the data-ak-details was actually updating, but only after React rendered the component. Since the data is in a DOM element and outside the React ecosystem, I needed a solution to monitor when the data attribute changed.
After some research I landed on using a MutationObserver, and connecting it to React via useRef and state. This seems to work well - it works both on initial page load, and on updates.
jeremylenz
force-pushed
the
ak-ui
branch
2 times, most recently
from
35b3b34
to
3a39a4b
Compare
|
🍏 |
|
rebased |
|
Suggested patch. This prevents an annoying UI error and worked well for me w.r.t to the angular pages and hammer ak update calls. I have not tried the react page. diff --git a/app/controllers/katello/api/v2/activation_keys_controller.rb b/app/controllers/katello/api/v2/activation_keys_controller.rb
index cd5b0ddecb..e976548207 100644
--- a/app/controllers/katello/api/v2/activation_keys_controller.rb
+++ b/app/controllers/katello/api/v2/activation_keys_controller.rb
@@ -304,7 +304,8 @@ module Katello
@content_view_environments = []
if params[:environment_id] || params[:environment]
find_cve_for_single
- elsif params[:content_view_environments] || params[:content_view_environment_ids]
+ elsif params[:multi_content_view_environment] != false &&
+ (params[:content_view_environments] || params[:content_view_environment_ids])
@content_view_environments = ::Katello::ContentViewEnvironment.fetch_content_view_environments(
labels: params[:content_view_environments],
ids: params[:content_view_environment_ids], |
|
Updated to bring back the CVE info card to the new AK details page. (Not sure how that went missing..)
|
...ack/components/extensions/HostDetails/Cards/ContentViewDetailsCard/ContentViewDetailsCard.js
Outdated
Show resolved
Hide resolved
| }), | ||
| })), | ||
| hostPermissions: PropTypes.shape({ | ||
| edit_hosts: PropTypes.bool, |
There was a problem hiding this comment.
needs to be tailored to work with AK
There was a problem hiding this comment.
In AK CVEDetailsBareCard is only for display, so we don't need to worry about permissions (yet).
| } | ||
| end | ||
| node :candlepin_name do |cve| | ||
| cve.candlepin_name |
There was a problem hiding this comment.
Is there a better name we can provide in the JSON for this? cve_label maybe? candlepin_name seems a little off if we are using the name for things we are doing in katello and hammer..
There was a problem hiding this comment.
It's also called just label..
I've always been fond of "candlepin_name" 😢 but I guess I can change it.
There was a problem hiding this comment.
Time to make candlepin_name katello_name since we are using it everywhere including asking users to use it in hammer..
I'll leave it to your better judgement..I know we have a story to make these candlepin_names more easily available to users..
There was a problem hiding this comment.
Looks good..Ack 👍🏼
One thing we can discuss more is the Multiple Content View Environment column on the AK table..It's not controlled by the multi-CV setting..Could be unnecessary information for single-CV setups but also a good way to familiarize users of the feature if they miss the RN.
I thought about this too.. But thinking about other places in the UI that we show multi-CV, those are not controlled by setting either. And if you have multi-env AK's/hosts and turn the setting off, they will still continue to work and show in the UI. |
|
Merging with a couple outstanding issues to be resolved in a followup:
|
What are the changes introduced in this pull request?
Display multiCV activation key details in the web UI. Does not include changing multiCV activation keys' content view environments.
ContentViewEnvironmentDetailscard into aCVEDetailsBareCardso we can reuse it. This was necessary because theContentViewEnvironmentDetailswas coupled to auseUrlParamswhich made it not reusable enough.content_view_environmentsnodes to activation key rablConsiderations taken when implementing this change?
I'm using
foreman-react-component(its first use in Katello) to render a React component on an AngularJS page. It's a bit hacky but it works!When an activation key is single-environment, we fall back to displaying what we always have, rather than displaying the new card. This should help with existing web UI automation tests.
What are the testing steps for this pull request?
Create one or more multiCV activation keys
Test both pages
make sure edits to other fields still work
display should work fine as well